Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSH config format to server list command #56

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PaddyT
Copy link

@PaddyT PaddyT commented Jan 13, 2023

I regularly find myself running a python script to update my ~/.ssh/config entries, whenever I create new servers on the Faculty platform. I thought it might be useful to contribute back and add it as a feature to the CLI.

Problem statment

Currently, if you create a new server and would like to access it remotely via SSH, you have to run a few separate commands:

1️⃣ Retrieve a list of your servers by name and (only with the verbose flag) their Project:

> faculty server list -v                                   
Project Name        Server Name       Type     Machine Type      CPUs  RAM    Status    Server ID            Started
My Project Name     server_name       jupyter  -                    1  4GB    running   some-server-id       2023-01-01 01:23

2️⃣ Retrieve an IP address for my server:

> faculty server ssh-details "Project Name" server_name
Hostname       Port  Username
12.34.56.78   12345  username

3️⃣ Copy-paste that IP address from my std output and then run my ssh command

> ssh [email protected] -P 12345

This is a little cumbersome for workflows where you create/update your servers often.

Solution

My proposed change is to modify the faculty server list command, adding an extra --format option which supports returning a list of servers in a format which can be piped directly to a user's ssh_config file.
This is super useful for workflows where you create/update your servers often, since it allows you to "save" your server ssh details and propagate that info to places like your IDE or command line client.

Example:

> faculty server list  --format ssh-config
Host server_name-project_name
    HostName 12.34.56.78
    User username
    Port 12345

I've removed the --verbose flag, and moved retrieving the full table of server info to --format table, since it seemed overcomplicated to have both "verbose" and "format" tags.

@imrehg
Copy link
Member

imrehg commented Jan 14, 2023

Changes from #57 should fix up the tests, just FYI.

@acroz
Copy link
Member

acroz commented Jan 16, 2023

Looks great, thanks for the contribution.

I'm wary of removing the -v flag with no deprecation period, and for the consistency of the CLI (which uses --verbose extensively elsewhere) I'm tempted to leave it in. Would you mind if we leave --verbose in, and error out in the case that both --verbose and --format are specified?

Separately, should we consider merging faculty server ssh-details into this? (with a warning on ssh-details that it's going to be removed)

@PaddyT
Copy link
Author

PaddyT commented Jan 19, 2023

I'm wary of removing the -v flag with no deprecation period, and for the consistency of the CLI (which uses --verbose extensively elsewhere) I'm tempted to leave it in. Would you mind if we leave --verbose in, and error out in the case that both --verbose and --format are specified?

Absolutely, makes sense. Done! The behaviour when you don't pass --format is now unchanged. Added test cases to make the new behaviour clear.

@@ -558,16 +575,36 @@ def list_servers(project, all, verbose):
server.created_at.strftime("%Y-%m-%d %H:%M"),
)
)
if not found_servers and verbose:

if not found_servers:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing behaviour deliberately only shows "No servers" in the verbose case, so that if you have a script consuming this output, it doesn't receive what looks like a single server called "No servers".

I think to keep existing behaviour we should have:

  • For the old logic not found_servers and verbose printing "No servers" - add as a nested if not found_servers under if format == "table"
  • For the old logic project and verbose printing a table with the first (project) column removed - as you've already implemented
  • For the old logic project or not verbose printing a list of server names - as you've implemented under else: (but without the short circuit this comment is attached to - no servers should result in empty output)
  • For the old logic not project and verbose printing a table printing a table including the first (project) column - as you've already implemented

Does this sound ok? Removing this short circuit would also result in the ssh-config format being valid in the case there are no matching servers.

return

if format == "table":
if project:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was missing from the current implementation, but could you include a comment that this is removing the project column from the output? It's not clear from the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants